Add some color to the filters and update occupancy links#689
Add some color to the filters and update occupancy links#689RyaliNvidia merged 3 commits intomainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughExposes an optional Changes
Sequence Diagram(s)sequenceDiagram
participant User as "User"
participant Table as "OccupancyTable\n(click handler)"
participant URLBuilder as "buildWorkflowsUrl"
participant Router as "Next.js Router"
Note over User,Table: Child row clicked
User->>Table: click child row
Table->>URLBuilder: buildWorkflowsUrl(row, groupBy, searchChips)
URLBuilder-->>Table: workflowsUrl
Table->>Router: router.push(workflowsUrl)
Router-->>User: navigates to workflows page
Note over User,Table: Parent row clicked
User->>Table: click parent row
Table-->>Table: toggle expansion state
Table-->>User: expansion toggled
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/ui/src/features/occupancy/components/occupancy-data-table.tsx (1)
116-133: Consolidate child workflow URL derivation to one source.
buildWorkflowsUrl(row, groupBy, searchChips)is repeated in both click and href paths. Extract a singlegetChildWorkflowsUrlcallback and reuse it to avoid drift.As per coding guidelines, "Single source of truth for each concept - avoid multiple representations of the same data."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/src/features/occupancy/components/occupancy-data-table.tsx` around lines 116 - 133, Create a single getChildWorkflowsUrl callback that returns buildWorkflowsUrl(row, groupBy, searchChips) for a child row and replace the duplicated calls in handleRowClick and getRowHref with this new callback; ensure getChildWorkflowsUrl is memoized with useCallback and includes groupBy and searchChips in its dependency array, then update handleRowClick (which uses onToggleExpand and router) to call router.push(getChildWorkflowsUrl(row)) for child rows and update getRowHref to return getChildWorkflowsUrl(row) for child rows, leaving parent behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ui/src/features/occupancy/components/occupancy-column-defs.tsx`:
- Around line 199-203: The child-row cell rendering replaced a semantic Link
with a non-semantic span (see the branch checking original._type === "child" in
occupancy-column-defs.tsx), which removes native link semantics and hurts
accessibility; restore a semantic link element (use the same Link component or
an anchor) for the child display so keyboard and screen-reader affordances are
preserved, ensure the Link includes the correct href/route and any existing
onClick/row-click navigation logic is preserved (transfer handlers or call the
row navigation from the Link's onClick) and keep the displayed text as
original.key so behavior and appearance remain the same.
---
Nitpick comments:
In `@src/ui/src/features/occupancy/components/occupancy-data-table.tsx`:
- Around line 116-133: Create a single getChildWorkflowsUrl callback that
returns buildWorkflowsUrl(row, groupBy, searchChips) for a child row and replace
the duplicated calls in handleRowClick and getRowHref with this new callback;
ensure getChildWorkflowsUrl is memoized with useCallback and includes groupBy
and searchChips in its dependency array, then update handleRowClick (which uses
onToggleExpand and router) to call router.push(getChildWorkflowsUrl(row)) for
child rows and update getRowHref to return getChildWorkflowsUrl(row) for child
rows, leaving parent behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3884a781-e5f6-4e73-87bc-542d9ac97e55
📒 Files selected for processing (6)
src/ui/src/components/data-table/data-table.tsxsrc/ui/src/components/data-table/virtual-table-body.tsxsrc/ui/src/components/filter-bar/filter-bar-dropdown.tsxsrc/ui/src/features/occupancy/components/occupancy-column-defs.tsxsrc/ui/src/features/occupancy/components/occupancy-data-table.tsxsrc/ui/src/features/occupancy/styles/occupancy.css
src/ui/src/features/occupancy/components/occupancy-column-defs.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ui/src/features/occupancy/components/occupancy-column-defs.tsx (1)
199-209:⚠️ Potential issue | 🟡 MinorVisual affordance missing for child row link—add hover and focus styling.
The
tabIndex={-1}is correct here: child rows are already keyboard accessible via the row itself (which receivestabIndex={0}whenonRowClickis provided). Both the row and the Link navigate to the same URL viabuildWorkflowsUrl(), so removing the link from the tab order avoids duplicate focus targets. ThestopPropagation()correctly prevents the row click from firing when the link is clicked directly.However, removing the hover/focus underline styling makes the link appear as plain text, reducing the visual affordance that it's interactive. Add back hover and focus-visible styles to maintain clarity for visual users:
Example styling
className="pl-2 text-sm text-zinc-600 dark:text-zinc-400 hover:underline focus-visible:underline"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/src/features/occupancy/components/occupancy-column-defs.tsx` around lines 199 - 209, The child-row Link rendering (inside the conditional checking original._type === "child") currently has tabIndex={-1} and loses hover/focus affordance; update the Link's className to include hover and focus-visible underline styles (e.g., add hover:underline and focus-visible:underline to the existing "pl-2 text-sm text-zinc-600 dark:text-zinc-400") so the link remains visually identifiable while keeping tabIndex={-1} and the onClick stopPropagation behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/ui/src/features/occupancy/components/occupancy-column-defs.tsx`:
- Around line 199-209: The child-row Link rendering (inside the conditional
checking original._type === "child") currently has tabIndex={-1} and loses
hover/focus affordance; update the Link's className to include hover and
focus-visible underline styles (e.g., add hover:underline and
focus-visible:underline to the existing "pl-2 text-sm text-zinc-600
dark:text-zinc-400") so the link remains visually identifiable while keeping
tabIndex={-1} and the onClick stopPropagation behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0ffa2f72-4df2-403f-9a7f-ab54c2f0e3e9
📒 Files selected for processing (1)
src/ui/src/features/occupancy/components/occupancy-column-defs.tsx
3 participants
Making the filter options be highlighted differently for better readability:
Making the links in the occupancy page nicercleaner. Clicking the row now redirects you instead of just the username + added tooltip:
Issue #None
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Style
UX